Skip to content

Add support list of strings#4

Closed
Suh. Junmin (LazyRichard) wants to merge 1 commit intoPSModule:mainfrom
LazyRichard:main
Closed

Add support list of strings#4
Suh. Junmin (LazyRichard) wants to merge 1 commit intoPSModule:mainfrom
LazyRichard:main

Conversation

@LazyRichard
Copy link

@LazyRichard Suh. Junmin (LazyRichard) commented May 5, 2025

Description

This PR supports base64 encoding of the results of functions that return as lists, such as Get-Content.

Rename the parameter to InputData, as in Out-File, Convert-ToJson because the input is now not a simple string.

Type of change

  • 📖 [Docs]
  • 🪲 [Fix]
  • 🩹 [Patch]
  • ⚠️ [Security fix]
  • 🚀 [Feature]
  • 🌟 [Breaking change]

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

Copilot AI review requested due to automatic review settings May 5, 2025 03:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the documentation to describe the new support for base64 encoding of list outputs, such as those returned by Get-Content.

  • Adds a new section for converting a file to base64 with an example.
  • Updates the expected output example to match the new multi-string input support.
Files not reviewed (2)
  • src/functions/public/ConvertTo-Base64.ps1: Language not supported
  • tests/Base64.Tests.ps1: Language not supported
Comments suppressed due to low confidence (1)

README.md:56

  • Consider adding a note or an additional test example to clarify how multi-line inputs (lists of strings) are handled during the base64 conversion to ensure the behavior is well documented.
Get-Content file.txt | ConvertTo-Base64

@LazyRichard
Copy link
Author

Marius Storhaug (@MariusStorhaug) could you review this?

@MariusStorhaug
Copy link
Member

Not sure I would implement it like this. The point you are making is to support pipeline inputs as single item and array, which is fine. The change you are suggesting is a breaking change, when it doesn't have to be. Are you fine if I take your branch and build it out without breaking changes? If you want to do this yourself: Think of the tests as a way to track breaking change. If you have to modify existing test (that is not due to bugs in tests) then its a breaking change.

I am not afraid of doing breaking changes as all module are following SemVer, however I do think this doesn't need to be a breaking change. I think we can make both -String and pipeline input available all at once.

Challenge accepted or want me to look at it?

@MariusStorhaug
Copy link
Member

Also sorry for being slow on this, for some reason I was not "watching" this repo, as in the built in watch => notification service.

... and thank you for the contribution :)

@LazyRichard
Copy link
Author

Suh. Junmin (LazyRichard) commented May 26, 2025

Thanks for review.
I don't know if this is what you wanted, but I tried to fix it.
Actually I'm not familiar with PowerShell. No matter what I did, I still have to change the data type, and in this case I think it's a bit awkward to have a parameter named String.

My First approach was using ParameterSetName.
I define parameter as String and InputObject but I have no luck.
because powershell doesn't distinguish between String and InputObject when invoke via pipeline.

any guide for this?

@MariusStorhaug
Copy link
Member

All good, just had AI do it for us :)

Implemented in the new version that is being built by Copilot as I write.

@MariusStorhaug
Copy link
Member

Thank you for opening the PR. Closing this now as its covered by the other RP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants